Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1741 Rolling camera and then interacting with the view provide incorrect interactions #1879

Merged
merged 11 commits into from
Jan 12, 2025

Conversation

samoncrief
Copy link
Contributor

A fix for the following bug: #1741

Copy link

codecov bot commented Jan 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.69%. Comparing base (5f7a99c) to head (cc789cb).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1879      +/-   ##
==========================================
+ Coverage   95.68%   95.69%   +0.01%     
==========================================
  Files         125      125              
  Lines        9961     9988      +27     
==========================================
+ Hits         9531     9558      +27     
  Misses        430      430              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Please add a test. A simple interaction test should do the trick

Copy link
Member

@Meakk Meakk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to understand the behavior with this PR. When the camera is rolled, the temporary up is used, but when do we restore the old up value? Only when we reset the camera?
I think we should slowing converge back the temporary up direction to the initial up direction by doing a slerp

library/src/interactor_impl.cxx Outdated Show resolved Hide resolved
@mwestphal
Copy link
Contributor

I'm trying to understand the behavior with this PR. When the camera is rolled, the temporary up is used, but when do we restore the old up value? Only when we reset the camera? I think we should slowing converge back the temporary up direction to the initial up direction by doing a slerp

Thats not what we want here I think.

@samoncrief
Copy link
Contributor Author

I'm trying to understand the behavior with this PR. When the camera is rolled, the temporary up is used, but when do we restore the old up value? Only when we reset the camera? I think we should slowing converge back the temporary up direction to the initial up direction by doing a slerp

The bug was that rotating the camera around the object while rolled causes the camera to jerk back to its original orientation. This PR does not change up in the renderer, it gives the interactor a definition to use after rolling with roll_camera that reflects the view up of the camera. When the camera is reset, the camera is oriented in a way where the renderer's up value is correct again, so at that point the flag is set to false.

@samoncrief samoncrief force-pushed the Roll-camera-view-interaction-bug branch from 43d07b6 to 449bc86 Compare January 6, 2025 16:57
@samoncrief samoncrief force-pushed the Roll-camera-view-interaction-bug branch from 449bc86 to f0a4eee Compare January 8, 2025 17:57
@samoncrief samoncrief force-pushed the Roll-camera-view-interaction-bug branch from d1b423d to 39305c5 Compare January 12, 2025 15:17
Copy link
Contributor

@mwestphal mwestphal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small style change then gtg

@samoncrief samoncrief force-pushed the Roll-camera-view-interaction-bug branch from 6680f6d to cc789cb Compare January 12, 2025 17:30
@mwestphal
Copy link
Contributor

Thanks for your contribution @samoncrief !

@mwestphal mwestphal merged commit 1fa15c3 into f3d-app:master Jan 12, 2025
47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants